Fix DTMF deduplication to use (timestamp, event code) composite key - #796
Fix DTMF deduplication to use (timestamp, event code) composite key#796cuihang wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #796 +/- ##
==========================================
+ Coverage 65.25% 66.70% +1.45%
==========================================
Files 51 42 -9
Lines 6588 8110 +1522
==========================================
+ Hits 4299 5410 +1111
- Misses 1915 2211 +296
- Partials 374 489 +115 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
94cae6a to
135a5fc
Compare
|
Hi @dennwc , just wanted to check if someone could take a look at this PR. We've hit this DTMF loss issue in production with our SIP carrier. All CI checks pass and the fix is minimal. Happy to make any adjustments if needed. |
dennwc
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you for the fix!
Some SIP devices or carriers may reuse the RTP timestamp of the previous digit for the next one, causing the current timestamp-only dedup to drop valid DTMF events. Change the dedup key from RTP timestamp to a composite (timestamp, event code) key, so that different digits sharing the same timestamp are still reported individually, while redundant packets of the same digit are still deduplicated. Also add a test case to verify the fix.
cac7746 to
20df4fe
Compare
The dtmf.Event struct and Encode function signatures changed in the newer media-sdk version. Update the new test to use dtmf.Write via rtp.Buffer instead of constructing raw DTMF events directly.
|
Hi @dennwc, just a quick update — I've rebased onto the latest main to resolve the merge conflicts (the dtmf handler had moved to |
Problem
The DTMF handler in
dtmfHandler(media_port.go) deduplicated incoming DTMF packets using only the RTP timestamp:RFC 4733 requires all packets of a given digit to share identical timestamps, so this correctly filters redundant packets within one digit. However, some SIP devices or carriers reuse the timestamp of the previous digit when sending the next digit. When this happens, the next digit is incorrectly filtered out and never reported.
Fix
Replace the timestamp-only dedup with a (timestamp, event code) composite key:
Behavior
*,0,1)Known limitation
If an upstream sends two identical digits (e.g. two
0s) with the exact same timestamp and event code, the(timestamp, event code)key alone cannot distinguish them. Resolving that case requires full DTMF lifecycle state tracking (marker bit, End bit, duration reset, sequence number gaps). That is a more complex enhancement beyond this fix; in the worst case (all fields identical) the receiver cannot distinguish the digits at all.Changes
pkg/sip/media_port.go: ReplacelastDTMFTimestamp atomic.Uint32withlastDTMFEvent atomic.Uint64; updatedtmfHandlerto decode first, then dedup on composite key.pkg/sip/media_port_test.go: Update field reference; addTestMediaPortDTMFSameTimestampverifying two different digits (0and1) sharing the same RTP timestamp are both reported.Test plan
TestMediaPortDTMF— all 12 existing subtests pass (digits 1/12/123 × loss none/first/last/middle)TestMediaPortDTMFSameTimestamp— new test passes: digits0and1with same timestamp → both reported as01